feat: warn when picking up .grype.yaml from CWD#3428
Conversation
Grype's config search includes the current working directory, so a hidden .grype.yaml (or .grype/config.yaml) in CWD is silently applied when you run grype. That's surprising if you wandered into a directory that happens to contain one, and it's hard to spot when debugging unexpected scan behaviour. Emit a WARN-level log on startup naming the picked-up file when the implicit CWD finder is what produced the config. Stay quiet when the user passed --config / -c or set GRYPE_CONFIG, and stay quiet when the config came from the user's home directory or an XDG location. Closes anchore#3427. Signed-off-by: ChrisJr404 <chris@hacknow.com>
willmurphyscode
left a comment
willmurphyscode
left a comment
There was a problem hiding this comment.
I am not sure this is the right approach. Does it make sense instead to hook into actual config parsing in Fangs or Clio and update the behavior there to return a warning if config values are changed?
That would have a couple of advantages:
- It would address the issue in other tools, not just grype
- If we changed how config file parsing works (e.g. add or remove support for toml, support
--configuration-fileas an alias for--config, etc.) we wouldn't need to coordinate across repositories.
| // It looks for both finder layouts that fangs uses by default: | ||
| // - ./.<appName>.<ext> | ||
| // - ./.<appName>/config.<ext> | ||
| func Detect(appName string, args []string, env func(string) string) string { |
There was a problem hiding this comment.
This just checks whether a file exists, not whether content is loaded from it. Do we want to log.Warn based on file names, or based on unexpectedly loading configs?
| // supportedExts mirrors viper.SupportedExts. We hard-code the list rather | ||
| // than depend on viper here so this package can be used without pulling the | ||
| // configuration loader stack into tests. | ||
| var supportedExts = []string{ |
There was a problem hiding this comment.
There are formats in here grype doesn't currently support (.ini for one), and we don't want to have to keep this in sync with the formats we support.
| // the bundled short form "-cfoo"). We intentionally accept false positives | ||
| // here over false negatives: if anything that looks like a config flag is | ||
| // present, suppress the warning. | ||
| func explicitConfigFlag(args []string) bool { |
There was a problem hiding this comment.
This duplicates flag parsing elsewhere. If we added --configuration as a valid option in Grype (or Fangs/Clio) we would have a bug here.
| return true | ||
| case strings.HasPrefix(a, "-c=") && len(a) > 3: | ||
| return true | ||
| case strings.HasPrefix(a, "-c") && len(a) > 2 && !strings.HasPrefix(a, "--"): |
There was a problem hiding this comment.
What is this case for? when does a have the prefix -c and the prefix --? I feel like I'm missing something here.
|
fangs already includes the list of config files being loaded: ... so a tool can detect if a config file in the current directory was used fairly easily. Syft has an explicit spot for this config to land that probably needs to get added to grype and then read. I'm pretty sure it can be a This PR should be adjusted to use this ☝️ |
3 participants
Closes #3427.
Grype's default fangs config search walks the current working directory before
~/.grype.yamland the XDG locations, so a hidden.grype.yaml(or.grype/config.yaml) sitting in CWD is silently applied. @joshbressers flagged this on the issue: "I'm not entirely comfortable having Grype read a hidden file in the CWD without any sort of warning." This change adds the warning.What this does
On startup, if grype is about to pick up a config file from CWD via the implicit finder, it logs:
The warning suppresses itself when the file came from anywhere explicit:
--config/-c <path>(and the--config=path,-c=path,-cpathshort forms)GRYPE_CONFIGenvironment variable~/.grype.yamlor any XDG config locationIt also stays quiet when there's no CWD config to warn about.
Where it lives
A small
cmd/grype/cli/internal/configwarnpackage owns the detection so the logic is unit-testable without the whole config loader stack. The detector uses the same extension list viper supports (yaml,yml,json,toml,properties, etc.) and checks both finder layouts that fangs uses by default:./.grype.<ext>and./.grype/config.<ext>.The hook fires from the existing
WithInitializersblock incmd/grype/cli/cli.go, right after the logger is hoisted into place, so the warning goes through the regular logging pipeline (respects--quiet, log file routing, redaction, etc.).Manual verification
Built locally and verified the four cases:
Subdir form (
.grype/config.yaml) also fires correctly.Tests
cmd/grype/cli/internal/configwarn/configwarn_test.gocovers: no file present, each supported extension, subdir layout, all--config/-c/--config=/-c=/-cpathforms,GRYPE_CONFIGenv, and a trailing--c-with-no-value edge case (treated as not explicit, so the warning still fires).go test ./...passes for the whole tree minus pre-existing docker-daemon-required integration tests in this environment (test/integration,test/cli,grype/pkgimage fixtures), which fail with "could not build docker image" unrelated to this change.go vet ./cmd/...is clean. The pre-existing vet warnings ongrype/db/v5/differ,grype/db/v6/installation, andgrype/versionare present onmainalready.Notes